Support client cert validation (spki, hash, san) by kinolaev · Pull Request #5868 · envoyproxy/gateway

kinolaev · 2025-04-30T10:12:53Z

What type of PR is this?

api: add publicKeyPins, certificateHashes and subjectAltNames fields to ClientTrafficPolicy.spec.tls.clientValidation
feat(translator): translate these fields to tls validation context

What this PR does / why we need it:
This PR allows to validate a client certificate against a list of SKPI hashes, certificate hashes or SANs using ClientTrafficPolicy

Which issue(s) this PR fixes:

#5909

Release Notes: Yes

arkodg · 2025-05-02T16:00:40Z

hey hoping to get to this one in 1.5 weeks post v1.4 release

codecov · 2025-05-02T16:08:28Z

Codecov Report

Attention: Patch coverage is 96.38554% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.93%. Comparing base (2dc3bf3) to head (11b890f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/listener.go	93.02%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5868      +/-   ##
==========================================
+ Coverage   70.84%   70.93%   +0.09%     
==========================================
  Files         220      220              
  Lines       37188    37259      +71     
==========================================
+ Hits        26345    26429      +84     
+ Misses       9295     9286       -9     
+ Partials     1548     1544       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kinolaev · 2025-05-02T16:21:37Z

Thanks @arkodg , sounds good! I fixed crds and will add some tests to make Codecov happy)

kinolaev · 2025-05-03T12:56:12Z

Hi @arkodg , sorry for bothering you, but I've added tests, could you please rerun the pipeline? I'd like to be sure that Codecov is happy)

Additionally I've created an issue #5909 with a design proposal for this PR.
I believe it fully complies with your contribution guide now)

kinolaev · 2025-05-03T15:08:35Z

gen-check test failed because of broken make generate in the main branch. I rebased my branch and ran make generate again, the test must succeed now.
I also added a release note.

zhaohuabing · 2025-05-19T02:37:06Z

api/v1alpha1/tls_types.go

Should we name it PublicKeyHashes?

PublicKeyPins may be better here though. I'm just speaking from the perspective of a non-native English reader.

I've chosen the name PublicKeyPins because the format base64(sha256(public key from cert in DER)) comes from Public Key Pinning Extension for HTTP rfc7469 where they have Public-Key-Pins HTTP header. But let me know if you still want me to rename it to PublicKeyHashes, I won't argue)

I'd go with PublicKeyHashes or SPKIHashes because:

It's easier to understand at a glance, without requiring background knowledge of Public Key Pinning Extension for HTTP rfc7469.

Even though we use the same format of Public-Key-Pins HTTP header here, the purpose is different.

cc @envoyproxy/gateway-maintainers please vote for

PublicKeyHashes

SPKIHashes

PublicKeyPins

My vote is either 1 or 2

I do like SPKIHashes, I've renamed the field. Ready to do it again if the vote suggest a different option)

zhaohuabing

Nit: Prefer to add yaml tests to the gateway API translator and xDS translator instead of individual go tests files.

zhaohuabing

It's not strictly necessary, but adding an e2e test for client cert validation would be helpful.

kinolaev · 2025-06-08T10:59:48Z

@zhaohuabing I've added testdata as you requested to:

internal/gatewayapi/testdata/clienttrafficpolicy-mtls-client-verification.in.yaml
internal/xds/translator/testdata/in/xds-ir/mutual-tls-san.yaml

Do I have to delete all or some of go tests from the files below?

internal/gatewayapi/clienttrafficpolicy_test.go
internal/gatewayapi/helpers_test.go
internal/xds/translator/listener_test.go

zhaohuabing · 2025-06-11T07:56:02Z

@zhaohuabing I've added testdata as you requested to:

internal/gatewayapi/testdata/clienttrafficpolicy-mtls-client-verification.in.yaml

internal/xds/translator/testdata/in/xds-ir/mutual-tls-san.yaml

Do I have to delete all or some of go tests from the files below?

internal/gatewayapi/clienttrafficpolicy_test.go

internal/gatewayapi/helpers_test.go

internal/xds/translator/listener_test.go

I think we wouldn't need clienttrafficpolicy_test.go and listener_test.go, but it's fine to keep them.

kinolaev · 2025-06-13T11:08:18Z

we wouldn't need clienttrafficpolicy_test.go and listener_test.go

Agree, these were duplicates of testdata, I've removed them.

zhaohuabing · 2025-06-15T14:08:26Z

api/v1alpha1/tls_types.go

Nit: consider renaming it to OtherSANNameMatch orOtherSANMatch ?
All API types are in the same namespace, a more specific name would be better.

SAN type OtherName is defined in section 4.2.1.6 Subject Alternative Name of rfc5280. Envoy and cert-manager follow this definition. That is why I'd prefer to not rename the field.

OK, OtherNameMatch works for me.

Oh, I'm sorry, only now I understand that you asked to rename the type and not the field! I agree I should definitely add "SAN" to the type name, I'll do it shortly.

Renamed to OtherSANMatch

zhaohuabing · 2025-06-24T02:14:46Z

LGTM.

@kinolaev Can you resolve the conflicts and fix the lint?

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

kinolaev · 2025-06-29T11:31:56Z

Thank you @zhaohuabing, the conflicts are resolved and CI is green now)

zhaohuabing

LGTM thanks!

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

kinolaev requested a review from a team as a code owner April 30, 2025 10:12

kinolaev force-pushed the tls-validation-context branch from f9b7336 to 0407b7e Compare April 30, 2025 10:16

kinolaev force-pushed the tls-validation-context branch from 0407b7e to 2ebdd7e Compare May 2, 2025 16:19

kinolaev force-pushed the tls-validation-context branch from 2ebdd7e to c8a342b Compare May 3, 2025 11:03

kinolaev mentioned this pull request May 3, 2025

Support client cert validation (spki, hash, san) #5909

Open

kinolaev force-pushed the tls-validation-context branch from c8a342b to e03472a Compare May 3, 2025 15:04

kinolaev force-pushed the tls-validation-context branch from e03472a to 1d129c8 Compare May 3, 2025 16:55

zhaohuabing mentioned this pull request May 19, 2025

feat: support listener name in SecurityPolicy target #5916

Merged

zhaohuabing reviewed May 19, 2025

View reviewed changes

kinolaev force-pushed the tls-validation-context branch 2 times, most recently from ef542b9 to 9d79c92 Compare June 8, 2025 11:29

kinolaev force-pushed the tls-validation-context branch 2 times, most recently from eaa17f3 to 0d87295 Compare June 13, 2025 11:06

zhaohuabing reviewed Jun 15, 2025

View reviewed changes

kinolaev added 5 commits June 29, 2025 14:26

feat: support client cert validation (spki, hash, san)

c4fe1ab

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

fix: mtls othername san struct

56a24b3

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

chore: add testdata for mtls san

1cf2bf6

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

chore: rm clienttrafficpolicy and listener tests

3780f1f

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

chore: rename publicKeyPins to spkiHashes

ade7e50

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

kinolaev added 2 commits June 29, 2025 14:26

chore: rename OtherNameMatch to OtherSANMatch

539f114

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

chore: fix testdata

a01c7e6

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

kinolaev force-pushed the tls-validation-context branch from 0d87295 to a01c7e6 Compare June 29, 2025 10:41

zhaohuabing previously approved these changes Jun 30, 2025

View reviewed changes

Merge branch 'main' into tls-validation-context

11b890f

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp dismissed zhaohuabing’s stale review via 11b890f June 30, 2025 03:16

rudrakhp approved these changes Jun 30, 2025

View reviewed changes

rudrakhp requested a review from zhaohuabing June 30, 2025 03:17

zhaohuabing approved these changes Jun 30, 2025

View reviewed changes

rudrakhp enabled auto-merge (squash) June 30, 2025 03:18

rudrakhp merged commit 1445be7 into envoyproxy:main Jun 30, 2025
31 checks passed

BrewTestBot mentioned this pull request Aug 8, 2025

egctl 1.5.0 Homebrew/homebrew-core#232799

Merged

Conversation

kinolaev commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented May 2, 2025

Uh oh!

codecov bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kinolaev commented May 2, 2025

Uh oh!

kinolaev commented May 3, 2025

Uh oh!

kinolaev commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaohuabing May 19, 2025

Choose a reason for hiding this comment

Uh oh!

kinolaev Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinolaev Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

zhaohuabing left a comment

Choose a reason for hiding this comment

Uh oh!

zhaohuabing left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinolaev commented Jun 8, 2025

Uh oh!

zhaohuabing commented Jun 11, 2025

Uh oh!

kinolaev commented Jun 13, 2025

Uh oh!

zhaohuabing Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinolaev Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

kinolaev Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

kinolaev Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

zhaohuabing commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinolaev commented Jun 29, 2025

Uh oh!

zhaohuabing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kinolaev commented Apr 30, 2025 •

edited

Loading

codecov bot commented May 2, 2025 •

edited

Loading

kinolaev commented May 3, 2025 •

edited

Loading

zhaohuabing Jun 11, 2025 •

edited

Loading

zhaohuabing left a comment •

edited

Loading

zhaohuabing Jun 15, 2025 •

edited

Loading

kinolaev Jun 15, 2025 •

edited

Loading

zhaohuabing commented Jun 24, 2025 •

edited

Loading